Skip to content

fix(newmm): resolve exponential BFS path explosion in ambiguous tokenization - #101

Merged
bact merged 3 commits into
mainfrom
copilot/fix-newmm-tokenizer-bfs-explosion
Apr 2, 2026
Merged

fix(newmm): resolve exponential BFS path explosion in ambiguous tokenization#101
bact merged 3 commits into
mainfrom
copilot/fix-newmm-tokenizer-bfs-explosion

Conversation

Copilot AI commented Apr 2, 2026

Copy link
Copy Markdown
Contributor

The newmm BFS graph traversal had O(2^n) worst-case behavior on highly ambiguous input — the same bug fixed in Python via PyThaiNLP/pythainlp#1319. Without a visited set, every neighbor is unconditionally re-enqueued, causing the queue to grow exponentially when the dictionary produces many overlapping matches.

Changes

  • bfs_paths_graph — add visited: HashSet<CharacterIndex>: Only enqueue nodes not yet seen. Reduces worst-case from O(2^n) to O(V+E).

    // Before: every neighbor unconditionally enqueued
    if position != goal {
        current_queue.push_back((position, appended_path));
    }
    
    // After: skip already-visited nodes
    } else if !visited.contains(&position) {
        visited.insert(position);
        current_queue.push_back((position, appended_path));
    }
  • one_cut commit-point branch — call graph.clear(): Previously only graph_size was reset to 0; stale edges from resolved segments accumulated across BFS calls. Graph is now cleared at each commit point.

  • one_cut no-candidate branch — remove unnecessary edge insertion: The non-dictionary-match branch inserted a graph edge that was never needed (token is yielded directly). Replaced with graph_size = 0; graph.clear(), consistent with the commit-point branch.

  • Regression test (test_newmm_ambiguous_performance): 250-character maximally ambiguous input (1–3 char overlapping words covering every TCC boundary) must complete within 1 s. With the fix it runs in ~10 ms.

- Add visited set to bfs_paths_graph to prevent re-exploring already-visited
  nodes, reducing worst-case BFS from O(2^n) to O(V+E)
- Clear ambiguity graph after each commit point in one_cut to prevent
  unbounded edge accumulation
- Remove unnecessary graph edge insertion in the no-candidate branch
- Add test_newmm_ambiguous_performance regression test
- Update CHANGELOG.md

Agent-Logs-Url: https://github.com/PyThaiNLP/nlpo3/sessions/b1879073-e5b6-44a8-8492-7eb13a0cb26c

Co-authored-by: bact <128572+bact@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a worst-case exponential blow-up in Newmm’s BFS-based ambiguity resolution by ensuring graph traversal doesn’t repeatedly re-enqueue already-seen nodes, and by clearing the ambiguity graph at commit points to avoid stale edges accumulating across segments.

Changes:

  • Add a visited set to bfs_paths_graph to prevent repeated exploration of the same positions.
  • Clear the ambiguity graph (graph.clear()) when committing a resolved segment and when handling the no-candidate branch.
  • Add a regression test that exercises highly ambiguous input and asserts it completes within a time bound.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
src/tokenizer/newmm.rs Prevents BFS queue explosion via visited tracking and clears graph state between committed segments.
tests/test_tokenizer.rs Adds a regression test for highly ambiguous input to guard against BFS path explosion.
CHANGELOG.md Documents the Newmm BFS/graph fixes and the newly added regression test.

Comment thread tests/test_tokenizer.rs Outdated
Comment thread tests/test_tokenizer.rs Outdated
Comment thread src/tokenizer/newmm.rs Outdated
Comment thread CHANGELOG.md Outdated
@bact

bact commented Apr 2, 2026

Copy link
Copy Markdown
Member

@copilot apply changes based on the comments in this thread

There are questions about claims on reduce complexity don't match the actual implementation. Recheck the implementation if anything is left unoptimized, or the claim is inaccurate.

- Fix doc comment: _bfs_paths_graph -> bfs_paths_graph in test
- Fix spelling: maximising -> maximizing (American English)
- Reword BFS comment: drop inaccurate O(V+E) claim; say each vertex
  is enqueued at most once, avoiding exponential blow-up
- Update CHANGELOG: replace O(V+E) claim with accurate description
  of avoiding exponential queue growth

Agent-Logs-Url: https://github.com/PyThaiNLP/nlpo3/sessions/b8052556-3598-419d-80e8-7af979d714e3

Co-authored-by: bact <128572+bact@users.noreply.github.com>

Copilot AI commented Apr 2, 2026

Copy link
Copy Markdown
Contributor Author

@copilot apply changes based on the comments in this thread

There are questions about claims on reduce complexity don't m...

Applied all four review suggestions in commit 190ce39:

  • tests/test_tokenizer.rs:370 — Fixed doc comment: _bfs_paths_graphbfs_paths_graph
  • tests/test_tokenizer.rs:383 — Fixed spelling: maximisingmaximizing
  • src/tokenizer/newmm.rs:281-282 — Replaced the inaccurate O(V+E) claim with: "The visited set ensures each vertex is enqueued at most once. This avoids the exponential blow-up from revisiting the same nodes." (Path cloning means true complexity is higher than O(V+E), but the key guarantee — no re-enqueuing — is accurately stated.)
  • CHANGELOG.md:18-19 — Same correction: removed O(V+E) claim and rephrased to "avoiding exponential queue growth".

@bact
bact marked this pull request as ready for review April 2, 2026 22:57
Copilot AI requested a review from bact April 2, 2026 22:59
Copilot finished work on behalf of bact April 2, 2026 22:59
@bact
bact merged commit 5263aa9 into main Apr 2, 2026
5 checks passed
@bact
bact deleted the copilot/fix-newmm-tokenizer-bfs-explosion branch April 2, 2026 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants